[Critical] Unify reduction construction, result recovery, and solver boundaries - #1151

Codecov Report

❌ Patch coverage is 95.15991% with 227 lines in your changes missing coverage. Please review.
✅ Project coverage is 96.49%. Comparing base (7dd5fcd) to head (6f65416).

@@            Coverage Diff             @@
##             main    #1151      +/-   ##
==========================================
+ Coverage   95.93%   96.49%   +0.56%     
==========================================
  Files        1074     1069       -5     
  Lines      132106   145001   +12895     
==========================================
+ Hits       126730   139914   +13184     
+ Misses       5376     5087     -289     

☔ View full report in Codecov by Harness.
📢 Have feedback on the report? Share it here.

Review of #1151

The recovery design is sound. Two reviewers read every hand-written recover_result and the shared helper, and I probed the CLI end to end. We found no path where a feasible-only target result or a backend failure becomes a source Infeasible or Optimal claim. ilp_qubo, the TSP→QUBO rewrite, the 3DM→3-partition fix, the maximal-matching rewrite and the petgraph swaps are all correct. Shared-prefix reuse in execute_paths is correct. No secrets, binaries, generated artifacts or debug leftovers in the diff.

The PR also had one regression that broke the NP-hardness chains, a few confirmed bugs, and several breaking changes the description does not mention. I fixed what I could and pushed the commits listed below (aaef6359..6f654162). The full gate passes on the result: make check, cargo clippy --workspace --all-targets --all-features -- -D warnings, the CLI tests with and without mcp, warning-free cargo build --lib with and without example-db, and the paper build with no "Missing reduction rules" block.

Fixed in the pushed commits

The 3-SAT chain to HamiltonianCircuit was cut. decisionminimumvertexcover_hamiltoniancircuit.rs moved its source to Decision<MVC<SimpleGraph, One>> while ksatisfiability_decisionminimumvertexcover.rs still targeted the i64 variant. pred path KSAT/K3 HamiltonianCircuit found no path, and HamiltonianPath, TSP, BottleneckTSP, LongestCircuit, QAP, RuralPostman, StackerCrane and the rest downstream of HC lost their witness chain from 3-SAT. examples/detect_unreachable_from_3sat compares problem names, so it missed this.

• 9ce1b155, 29960948, 1838a7bc KSAT/K3 now targets the One variant, which is what it builds. A new decisionminimumvertexcover_casts.rs adds One → i64 so ComparativeContainment stays reachable. extract_type_name in the macro crate now unwraps Type::Group, which a $ty:ty forwarded through macro_rules! produces. A variant-level reachability test covers both targets. The paper has the new cast entry.

Bugs.

• 116a2a30 UndirectedFlowLowerBounds accepted requirement = -5. The assert-to-Err rewrite turned requirement >= 1 on an i64 into == 0. The CreateSpec repeated every try_new check, which is why the bug existed twice. It now delegates to try_new.
• 4b021e5b, dcff030f Five models still loaded JSON without validation. PathConstrainedNetworkFlow with paths: [[99]] deserialized and then panicked with an index out of bounds. The others were MixedChinesePostman, ConsecutiveOnesMatrixAugmentation, MaximumContactMapOverlap and MonochromaticTriangle. The last one trusted serialized derived data, and pred create MonochromaticTriangle --graph ... failed with "missing field triangles". The JSON shapes did not change.
• 0546e886 ILPCoefficient for f64::from_integer became value as f64. It returns InexactFloatConversion beyond 2^53 again, as on main and as the numeric contract requires. The exact f64 comparison stays as you wrote it.
• c8e25e41 The clippy command in the description failed at head on cli_tests.rs:10298 with single_element_loop.
• ce5a8eda A default-features cargo build --lib printed 24 warnings: 22 unused SolveOutcome imports plus dead code in ksatisfiability_bicliquecover.rs.

Sum and And work as Problem::Value again. The Aggregate fold is a core contract here and the maintainer wants both types kept. The new Problem::Value: EvaluationValue bound locked them out, so type Value = Sum<u64> no longer compiled. #1148 asked that aggregate-only Sum uses remain usable.

• 1b4d9ce4, c24119aa, d182327b Problem::Value is Clone again. EvaluationValue is required where candidate feasibility is asked: ReductionResult::{Source, Target}, recover_preserving_status, SolutionAggregate, OptimizationValue, and declare_variants! registration. That is 7 sites in 5 files, and no rule or model file changed. New tests fold a Sum<u64> count and an And value with brute force and count evaluations to prove the is_absorbing break. A compile_fail doctest shows a Sum-valued problem cannot enter SolveOutcome::optimal. design.md, CLAUDE.md and three skills now describe this state.
• Removing ReduceToAggregate and extract_value is accepted. On main those served only as thresholds, and recover_result with Decision<P> targets does that job better. Please record the deviation on [Refactor] Treat each reduction as one construction and witness-mapping lifecycle #1148.
• 675711d8 and b3758460 are a delete and its revert from my side. They cancel out.
• add-rule/SKILL.md documents recover_preserving_status and not yet recover_by_source_evaluation. A short paragraph there would help rule authors.

QUBO JSON. a94c93c8 The persisted format was sprs's own CSR serde layout, which ties the wire format to a dependency, and files in main's dense format failed with invalid type: sequence, expected string or map. QUBO now writes {"num_vars":3,"entries":[[0,0,3],[0,1,-5],...]} in row-major order and still reads {"num_vars", "matrix"}. Round trips keep the f64 summation order bit for bit.

pred extract.

• c096a003 The help still described --config. It now shows the result JSON, explains the three statuses, and says to use feasible unless the solver proved optimality.
• 191d4dfb A bare configuration array failed with invalid type: integer 0, expected variant identifier. The error now states the expected shape and how to wrap a legacy array.
• e1367c89 A registry build failure was reported as "cannot inspect brute-force coordinates".
• 7f5a3090 The paper printed pred extract bundle.json --config ..., which now fails. reduction-workflow.typ still said extract_solution.
• a1ee27d2 Two skills linked design.md#witness-and-aggregate-reductions, which does not exist. Three still named extract_solution.
• 19b2547a InsufficientSolutionQuality named no rule. Errors that come through a chain or the CLI now use InsufficientSolutionQualityAt { source_problem, target_problem }, so the message reads MaximumSetPacking -> QUBO: the target result does not establish .... Direct typed recover_result calls still return the unit variant, and is_insufficient_quality() matches both. When pred extract reports source infeasible from an external optimal or infeasible claim, it prints a one-line note on stderr. stdout stays byte-identical and --quiet suppresses the note.

Tests for the branches this PR is about. 0e7e9721 Thirteen rules returned InsufficientSolutionQuality with no test in their own file. Each has one now. ilp_qubo, travelingsalesman_qubo and the inverse-kinematics rule also assert that an infeasible source recovers Infeasible through recover_result. They enumerate all target assignments (2^5, 2^16, 2^6) and decode them independently of the rule.

Duplication. 41a24634 Nine rules carried the same 28-line body: map the solution, evaluate the source, and let a target optimum with a source-invalid decoding prove source infeasibility. They now call recover_by_source_evaluation, which sits next to recover_preserving_status and states its premise in the rustdoc. I checked the premise against each construction and added a one-line justification where a rule lacked one. Twenty more bodies were plain status-preserving matches and now call recover_preserving_status. Net 418 lines removed under src/rules, and no test expectation changed. I left paintshop_qubo.rs and VariantReductionResult alone. The second would need Solution: Clone on a public impl.

CI. 6f654162 ci.yml and make clippy linted only the root package with example-db, which is how the CLI lint error and the unverified rmcp 3 migration got through green CI. Both now run cargo clippy --workspace --all-targets --all-features -- -D warnings. The features are all pure Rust, so the runner needs nothing new.

Please handle

1. Seal SolveOutcome. Its variants and fields are public and it derives Deserialize, so the same type is both the wire format and the value that is supposed to mean "this witness was validated". A caller can write SolveOutcome::Optimal { solution: all_false, evaluation: Or(true) }, and recover_result panics at coloring_qubo.rs:61. The same shape is in minimummultiwaycut_qubo.rs:74, minimumdiscreteplanarinversekinematics_qubo.rs:94, travelingsalesman_qubo.rs:86,111, ksatisfiability_preemptivescheduling.rs:353,364 and minimumvertexcover_ensemblecomputation.rs:63. Keep a plain serde DTO for the wire and make the typed outcome constructible only through optimal() and feasible(). The lean unwraps in the decoders are then justified by the type. The CLI path is safe today because it validates first.
2. Give InsufficientSolutionQuality a reason. It now names the edge. It still cannot say whether the rule needs a proven optimum or the sample broke a penalty constraint. This fits into the refactor in item 1.
3. Update the description. It should list:
   • the removal of SteinerTreeInGraphs and its ILP rule (this closes SteinerTree vs SteinerTreeInGraphs #722, so reference it)
   • the removal of ClosestVectorProblem<f64> (reference Separate CVP integer and floating-point variants under the numeric contract #1146, which asked to separate the variants, and say why the float variant is gone)
   • the removal of KColoring<K1/K4/K5>, of KSatisfiability → MinimumVertexCover, and of the getters total_tuples, domain_size_product, num_window_choice_product, lcm_moduli and Decision<MVC>::k()
   • the good_lp → highs = "=2.4.0" swap and the removal of ordered-float
   • the exact ILP<_, f64> comparison, with the flipped assertion in float_constraints_use_float_arithmetic
   • the extract output change ("solver":"external" became {"kind":"external"}, reduced_to dropped)
   • the QUBO JSON format
   • the rewrite of 7 skill files. The description says skills are excluded. The rewrite is accepted, including dropping the 5000-check and 3-test floors.
4. Migration notes and a version bump. The public API breaks in many places and the crate is still 0.6.0: extract_solution → recover_result, BruteForceProblem::dimensions() → num_variables() / dimension(i), TruthTable constructors returning Result, and the removal of solvers::decision_search, registry::ProblemMetadata, ReductionMode::Aggregate, AggregateReductionChain and SolveError::SearchSpaceOverflow.
5. Rejection tests for the new deserialization checks. 65 of the 107 newly validated models have none, and most of the rest assert only is_err(). 26 files repeat the try_new checks in TryFrom<CreateSpec>. That is what makes the three Codecov-flagged branches unreachable. Delegating to try_new, as 116a2a30 does for one model, fixes both.

Questions

• SteinerTree<_, i64> now accepts negative weights, so its complexity became 2^num_vertices * 0.5^num_terminals * num_vertices^2. Garey and Johnson define the problem with positive weights, where Dreyfus-Wagner applies. Is the generalization wanted?
• RuntimeKColoringCreateSpec rejects k == 0. KColoring::<KN>::with_k(graph, 0) and serde accept it, and a test asserts that. Which is intended?
• PathConstrainedNetworkFlow::try_new checks neither nonnegative capacities nor the sign of requirement. Its path errors print "problem construction failed:" twice, because one ConstructionError is formatted into another.
• QUBO::from_matrix and from_sparse store lower-triangle entries that evaluate never reads. The cli-commands.md example --matrix '1,0.5;0.5,2' is symmetric, so the user gets a 0.5 coupling and not 1.0. Should lower-triangle input be rejected or folded?
• Several error types put the cause in Display and also return it from source(), so anyhow prints it twice: "problem evaluation failed during recovery: X" and then "Caused by: X". See rules/traits.rs:11-16,31-36,141, rules/graph.rs ~158 and 190-212, solvers/mod.rs:32-58, solvers/ilp/solver.rs:42 and registry/variant.rs:120.
• register_decision_variant! for MVC passes random and also registers the One variant through additional:, but impl_random_generate! exists only for i64. Does pred create DecisionMinimumVertexCover/One --random advertise something it cannot do?

Smaller notes

• ReductionChain::recover_result::<S, T> never checks T against the path's real target. src/unit_tests/rules/graph.rs:558-573 passes the wrong target type and succeeds.
• pred extract trusts an external {"status":"infeasible"} even for a target that can never be infeasible, such as MIS.
• ExecutedStep { witness } is a one-field wrapper left from the two-channel design. check_reported_evaluation is 57 lines that validate an optional field whose value is then recomputed.
• The scripted assert-to-Err rewrite left 165 if !(, six if !(!x) and 128 stray }; lines under src/models.
• 22 FieldInfo { description: "" } entries in the new Decision registrations print blank lines in pred show.
• map_value in ilp_qubo.rs:86 and travelingsalesman_qubo.rs:99 computes a full source value, and production code only calls .is_valid() on it. minimumvertexcover_ensemblecomputation.rs:63 and ksatisfiability_preemptivescheduling.rs:353 use as casts.
• cli-config in the paper encodes JSON with pretty: true, so about 100 pred evaluate --config '[...' snippets span many lines in the PDF. Main does the same.
• docs/src/static/reduction-workflow.typ is referenced nowhere. Delete it or link it.
• Cargo.lock is gitignored, so CI resolves dependencies fresh on every run. CI still never runs the MCP tests. They pass locally after the rmcp 3 migration.
• Every dependency minimum moved to the newest patch (serde = "1.0.229" and so on). For a published library that constrains downstream resolvers for no functional gain.

At 1041 files this PR cannot be bisected. It mixes an API migration, dependency major bumps, a storage change, serde validation, model removals and a bug fix. The dependency commit and the 3-partition fix could have landed on their own.

Review boundary

Main-based changes without the core API migration are available in:

• Fix combinatorial reduction boundary cases and witness recovery #1155: combinatorial reduction boundary cases and witness recovery
• Refactor the website around a Typst-backed reduction graph #1156: website
• Simplify internal graph checks and Steiner witness extraction #1158: internal graph checks and Steiner extraction
• Validate graph, set-system, and database deserialization #1161: graph, set-system, and database input validation
• Validate flow and postman model inputs #1162: flow and postman input validation
• Maintain dependencies and workspace lint coverage #1163: dependencies and CI maintenance
• Validate matrix and numeric model deserialization #1164: matrix and numeric input validation
• Validate string and scheduling model inputs #1166: string and scheduling input validation
• Validate symbolic and compiler model inputs #1168: symbolic and compiler input validation
• Validate structured graph and planning model inputs #1169: structured graph and planning input validation

The local combined tree passes 6,572 workspace all-feature tests (2 ignored), all-target/all-feature Clippy, and 3 website build-contract tests. Combined changed-line Rust coverage is 99.64%.

The core review retains reduction/result recovery and solver interfaces, aggregate/counting semantics, public generic-bound changes, CVP/QUBO representations and numeric boundaries, graph path capabilities, and mathematical construction changes requiring separate justification. In particular, the remaining generic deserialization changes narrow public trait implementations, and MaximumLikelihoodRanking needs a consistent comparison-count range contract.

No PR has been merged to main. This PR still contains the extracted changes until the main-based PRs are merged and its base can be updated; no force-push or API migration was performed during extraction.

Closing this omnibus PR in favor of the independently reviewable main-based PRs and a separate [Critical] PR for the remaining API changes. The source branch is retained; this closure does not merge or discard its commits.